Implement system macro make_timestamp #988

nirosys · 2025-07-10T00:58:03Z

Issue #, if available: #942 (and a little of #970)

Description of changes:
This PR implements the make_timestamp system macro. It's a bigger PR than I really wanted, but I also wanted something to handle the timestamp builder a little better.

There are a few changes in here that are not directly related to make_timestamp, but do get used in the implementation:

Added an implementation of Sub for Decimal. I originally added this thinking some arithmetic operations on Decimals would make handling the fractional second Decimal nicer. Ultimately I didn't use it.
I added fract and trunc to Decimal to handle extracting the whole and fractional values from the decimal. This mirrors the floating point types in Rust.
I extended the SystemMacroArguments type to be a little more generic. It takes a generic argument that is AsRef<str>, which lets you provide a type that can be used to return a &str for the parameter name. In the make_timestamp implementation this is used to provide an enum that can be used to provide both the argument's position, as well as the name. Right now the only 2 implementations that are using it are make_decimal and make_timestamp, so it seems a bit overkill, but I'll circle back around and revisit the other system macros where it could be used.

The TimestampBuilderWrapper type that I'm using to handle the changing TimestampBuilder types adds a lot of bulk to the diff and I'm not too thrilled about that.

Conformance Test

Conformance tests for make_timestamp are passing:

Running tests: ion-tests/conformance/system_macros/make_timestamp.ion
  make_timestamp can be invoked                                          ...  [Ok]
  make_timestamp produces a single, unannotated timestamp                ...  [Ok]
  the arguments may have annotations, which are silently dropped         ...  [Ok]
  the year argument                                                      ...  [Ok]
  the month argument                                                     ...  [Ok]
  the day argument                                                       ...  [Ok]
  the hour argument                                                      ...  [Ok]
  the minute argument                                                    ...  [Ok]
  the hour and minute arguments are encoded in binary as tagged values   ...  [Ok]
  the seconds argument                                                   ...  [Ok]
  the offset argument                                                    ...  [Ok]

CLI

I have a local edit of the ion-cli to work with ion-rust HEAD. So I've been able to test the newer system macros that weren't part of rc6:

echo '$ion_1_1 (:make_timestamp 1 2 3 4 5 6.05 272)' | target/release/ion dump
0001-02-03T04:05:06.05+04:32

This has been super helpful.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… for fractional seconds

codecov · 2025-07-10T01:01:47Z

Codecov Report

Attention: Patch coverage is 82.31047% with 49 lines in your changes missing coverage. Please review.

Project coverage is 78.54%. Comparing base (79d6e05) to head (55540bb).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lazy/expanded/macro_evaluator.rs	81.52%	11 Missing and 35 partials ⚠️
src/types/decimal/mod.rs	92.00%	2 Missing ⚠️
src/lazy/expanded/template.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #988      +/-   ##
==========================================
+ Coverage   78.47%   78.54%   +0.07%     
==========================================
  Files         138      138              
  Lines       33385    33648     +263     
  Branches    33385    33648     +263     
==========================================
+ Hits        26198    26429     +231     
- Misses       5179     5183       +4     
- Partials     2008     2036      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

popematt · 2025-07-10T22:40:15Z

src/lazy/expanded/macro_evaluator.rs

+        // We have a value, if it is anything other than Day then it is invalid.
+        if arg.0 != MakeTimestampArgs::Day {
+            return IonResult::decoding_error(format!("value provided for '{parameter}', but no day specified."));
+        }


How could this be reached?

By invoking make_timestamp without a value for day, but providing a value for a parameter later:

(:make_timestamp 2025 1 (:none) 5)

Each known parameter has an entry in the expander's argument iterator, so we have to evaluate each parameter to make sure they resolve to an empty value to consider them not provided.

popematt · 2025-07-10T22:41:26Z

src/lazy/expanded/macro_evaluator.rs

+            .filter(|v| *v >= 1 && *v <= 31)
+            // Spec says 1 indexed day for the given month; does this mean we accept any integer
+            // >=1? or does the spec expect us to validate that the day is appropriate for the
+            // month and year?
+            .ok_or_else(|| IonError::decoding_error("value provided for 'Day' parameter is out of range [1, 31]"))?;


I think we should leave this sort of validation for Timestamp and/or TimestampBuilder to handle so that we don't have duplicated logic.

Timestamp and TimestampBuilder currently do not do any range checking, and the spec for make_timestamp differs from what is possible with the timestamp encoding.

Mostly around the year, where a long form timestamp can handle a year up to 2^14 (If I'm not misreading the spec), but make_timestamp is limiting years to the range 0-9999.

The question of whether we check "max day of any month" vs, checking "is this day valid for this month and year" should also then be raised to see if we want that kind of check added to the Timestamp/TimestampBuilder.

I'd have no problem with validating in the timestamp creation, but I don't think we'd want the checks to artificially limit the dates representable by timestamp.

The spec limits the year range to be 0..=9999 for Timestamp (indirectly by mentioning that it follows the W3C note on date and time formats) , and so Timestamp should only be able to represent valid points in time in that range (but forgetting about leap seconds and that sort of nonsense).

Timestamp and/or TimestampBuilder should have range checking, even if it is done indirectly somehow. I would recommend omitting the checks here, and creating an issue to go back and make sure that the TimestampBuilder cannot be used to construct an invalid Timestamp.

Ah good point. Yea, I like that. I'll remove the range checks and plan to add them to timestamp or timestamp builder asap.

Just realized there is range checking in the Timestamp, on build(), but it seems to be done through chrono.

Removing the range checking in the macro impl does lead to odd error behaviors in some cases. For instance, most arguments to the macro come in as Int, and when creating a timestamp they need to be converted down to a u32. So when converting I'll have to emit errors for not being able to convert to a u32, without really checking the range and providing a more context-aware error.

Doesn't seem like a big deal, just a bit weird.

popematt · 2025-07-10T22:48:26Z

src/lazy/expanded/macro_evaluator.rs

+                // Correct with the exponent jic the value is normalized to 5x10^1 or something.
+                let whole_seconds_i64 = whole_seconds_i64 * 10i64.pow(whole_seconds.exponent() as u32);
+
+                let builder = builder.clone().with_second(whole_seconds_i64 as u32);


Do we really need to clone() the builder here? If so, can you add a note as to why?

popematt · 2025-07-10T22:50:34Z

src/lazy/expanded/macro_evaluator.rs

+
+        // Check if we have a value.
+        let Some(value_ref) = arg.try_get_valueref(env)? else {
+            return Ok(self.clone());


Why clone() the builder-wrapper? Could we just make this function take self instead of &self so that it can return the original? Or perhaps &mut self so that it could swap out the value?

Yea, I made it a &self, because I wasn't sure how the process functions would end up being compiled, and kept the move-oriented API of the underlying builder. My understanding is that a non-ref will result in a memcpy when crossing function boundaries (if not inlined).

Going the &mut self route might be more straight forward. Since the type isn't changing, there's no reason to invalidate the pre-process value.

Will change that in a new commit, ty!

…ove range checks

nirosys added 4 commits July 8, 2025 17:54

Implement make_timestamp system macro

d51c9b0

Add trunc and fract to Decimal and migrate make_timestamp to use that…

f694d9a

… for fractional seconds

Update comment; Return error, even though it shouldn't be reachable

c7bace9

Add comment to fract

952a418

nirosys added 2 commits July 10, 2025 13:05

Fix decimal handling of whole seconds; add test case

c4158e3

Remove unused code; expand unit tests

75aaeb0

nirosys marked this pull request as ready for review July 10, 2025 20:50

nirosys mentioned this pull request Jul 8, 2025

Conformance tests failing due to unimplemented system macros #942

Open

7 tasks

popematt reviewed Jul 10, 2025

View reviewed changes

nirosys added 2 commits July 14, 2025 14:40

Address PR feedback; Move to mutable TimestampBuilderWrapper, and rem…

c498962

…ove range checks

Read month as u32, instead of i64 and downconverting

55540bb

nirosys requested a review from popematt July 16, 2025 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement system macro make_timestamp #988

Implement system macro make_timestamp #988

nirosys commented Jul 10, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jul 10, 2025 •

edited

Loading

Uh oh!

popematt Jul 10, 2025

Uh oh!

nirosys Jul 11, 2025

Uh oh!

popematt Jul 10, 2025

Uh oh!

nirosys Jul 11, 2025

Uh oh!

popematt Jul 11, 2025

Uh oh!

nirosys Jul 11, 2025

Uh oh!

nirosys Jul 12, 2025

Uh oh!

popematt Jul 10, 2025

Uh oh!

popematt Jul 10, 2025

Uh oh!

nirosys Jul 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

Implement system macro make_timestamp #988

Are you sure you want to change the base?

Implement system macro make_timestamp #988

Conversation

nirosys commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conformance Test

CLI

Uh oh!

codecov bot commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirosys Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nirosys commented Jul 10, 2025 •

edited

Loading

codecov bot commented Jul 10, 2025 •

edited

Loading

nirosys Jul 11, 2025 •

edited

Loading